Migrate to VM v0.22 and protocol v0.14#995
Conversation
d59d8d4 to
3a01c88
Compare
|
The integration tests are to be migrated after 0xMiden/miden-client#1891 is ready/merged. |
dcf2c50 to
99777a4
Compare
12bd9e8 to
bb280a8
Compare
|
@bitwalker @mooori I'm mostly finished with this PR. I'm combing through it to do some cleanups and I'm waiting for the client/node PR updated with released protocol crates. I suggest to review it on a per-commit basis, skipping non-important commits. |
0183581 to
9999250
Compare
Migrate the compiler workspace from miden-vm v0.20 (crates.io) to
v0.21.1 (local checkout) and miden-base v0.13 to v0.14.
Key breaking changes addressed:
- Remove FieldElement/StarkField traits (now inherent methods on Felt)
- .as_int() -> .as_canonical_u64() across all crates
- miden_core::Program -> miden_core::program::Program
- miden_core::AdviceMap -> miden_core::advice::AdviceMap
- miden_core::utils::{Serializable,Deserializable} -> miden_core::serde::*
- miden_assembly::utils::Deserializable -> miden_assembly::serde::*
- Felt::ELEMENT_BYTES/read_from_bytes/elements_as_bytes removed
- Felt::inv() -> Field::try_inverse()
- StackOutputs::get_stack_item() -> get_element()
- miden_processor::AdviceInputs -> miden_processor::advice::AdviceInputs
- MASM: u32overflowing_mul -> u32widening_mul
- MASM: u32overflowing_madd -> u32widening_madd
- Breakpoint instruction removed (mapped to Nop)
- NoteInputs -> NoteStorage in miden-protocol v0.14
- Felt::from(bool) removed
- SmtLeaf::to_elements() now returns iterator (needs .collect())
use re-exported trait from the `miden-field`.
9999250 to
95ef09a
Compare
eb7e4a9 to
d621f0a
Compare
|
@bitwalker @mooori I finished the new VM v0.22 migration and switched to the released crates. It's ready to be merged. EDIT: The only git deps left are |
| diagnostics.diagnostic(Severity::Error).with_message(message).into_report() | ||
| })?; | ||
|
|
||
| if cross_ctx_export_sig_flat.params().iter().any(|param| param.ty.is_pointer()) { |
There was a problem hiding this comment.
This turns a previously supported #[component] export shape into a hard failure IIUC?
There was a problem hiding this comment.
I believe this is handling the case where specific heap-allocated types that are valid in the Canonical ABI (such as list<T>), but which we do not yet support, are being passed across the component boundary. In the Wasm Component Model, it is left to the Wasm runtime to copy those types from the memory of the caller component to the memory of the callee component - and for our purposes we must emit code which uses the advice provider for that. IIUC, this is just raising a diagnostic to notify the user that support for such types are not yet implemented.
I do think the diagnostic message is misleading though (unless I'm mistaken about why this check is being performed here - but we can't ever allow arbitrary references to caller memory to be passed across a component boundary, with the exception I noted above). It might be better to elaborate on what specifically the issue is (i.e. is it a matter of using something like list<T> when we haven't implemented support for that Canonical ABI type yet - or is it an attempt to pass a pointer across the component boundary, which we will never support)
There was a problem hiding this comment.
I changed the error message in da45c32.
Yes, this is about passing the call parameters through the advice provider which we are not yet supporting.
frontend/wasm/src/component/flat.rs
Outdated
| } | ||
| Type::F64 => return Err(CanonicalTypeError::Reserved(ty.clone())), | ||
| Type::Felt => vec![AbiParam::new(Type::Felt)], | ||
| Type::Enum(enum_ty) => flatten_type(context, enum_ty.discriminant())?, |
There was a problem hiding this comment.
Do flattening and wrapper generation now disagree on enums?
It looks like any import/export that contains an enum inside a transformed struct/result will get through flattening and then fail during wrapper generation. Should we either teach load/store to handle enums the same way, or keep rejecting enums here until that path exists?
There was a problem hiding this comment.
Agreed, this flattens the enum type to its discriminant type here (so we should at least have assert!(enum_ty.is_c_like()) to catch the as-yet-unimplemented full breadth of the enum type in the Canonical ABI) - but then the load and store implementations in canon_abi_utils return an error, rather than matching the behavior here (i.e. by assuming that the enum is C-like, and thus that we're loading/storing the corresponding discriminant type).
|
FYI |
codegen/masm/src/emit/unary.rs
Outdated
| // Zero-extending a u64 to u128/i128 requires adding an extra 0u64 *above* the | ||
| // existing u64 in significance, i.e. below it on the operand stack (LE limb order). |
There was a problem hiding this comment.
| // Zero-extending a u64 to u128/i128 requires adding an extra 0u64 *above* the | |
| // existing u64 in significance, i.e. below it on the operand stack (LE limb order). | |
| // Zero-extending a u64 to u128/i128 requires us to provide the most-significant bits of the | |
| // resulting 128-bit value. This is acheived by pushing a `0u64` value _below_ the current u64 | |
| // value on the operand stack, as the limb order of multi-limb integer values is little-endian | |
NIt: I find the wording of this awkward, this is how I'd phrase it instead
codegen/masm/src/lower/lowering.rs
Outdated
| impl HirLowering for hir::Breakpoint { | ||
| fn emit(&self, emitter: &mut BlockEmitter<'_>) -> Result<(), Report> { | ||
| emitter.emit_op(masm::Op::Inst(Span::new(self.span(), masm::Instruction::Breakpoint))); | ||
| emitter.emit_op(masm::Op::Inst(Span::new(self.span(), masm::Instruction::Nop))); |
There was a problem hiding this comment.
Let's remove this operation entirely from the hir dialect
bitwalker
left a comment
There was a problem hiding this comment.
I spent most of this afternoon going through this, and it looks good! I did have a few nits/tweaks that I'd like to make before merging, but just ping me for another pass once those are settled and we can get this merged.
| /// Identifies whether canonical ABI flattening is being performed for an exported component | ||
| /// function or for a lowered import wrapper. | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||
| pub enum CanonicalAbiMode { |
There was a problem hiding this comment.
I would clarify that what's actually happening here, IMO the terminology should work like this:
Lift- means we are "lifting" a core Wasm function into the Canonical ABI (i.e. we're synthesizing aCallConv::ComponentModelfunction that will be exported, which internally calls the underlying function). The implementation of that function is "lowering" (flattening) arguments into the underlying ABI, and "lifting" results into the Canonical ABI.Lower- means we are "lowering" a Canonical ABI function into the underlying ABI (i.e. we're synthesizing aCallConv::Wasmfunction to call the Canonical ABI function). The implementation of that function is "lifting" arguments into the Canonical ABI, and "lowering" (flattening) results into the underlying ABI.
Better yet, it might make more sense to just change the names here to Export and Import to make it clearer what semantic action is being performed, and just document the WAT syntax it maps to (i.e. (canon lift) or (canon lower)).
Regardless, the terminology and semantics of this is very subtle, and I'd like to make it crystal clear what they map to.
frontend/wasm/src/component/flat.rs
Outdated
| } | ||
| Type::F64 => return Err(CanonicalTypeError::Reserved(ty.clone())), | ||
| Type::Felt => vec![AbiParam::new(Type::Felt)], | ||
| Type::Enum(enum_ty) => flatten_type(context, enum_ty.discriminant())?, |
There was a problem hiding this comment.
Agreed, this flattens the enum type to its discriminant type here (so we should at least have assert!(enum_ty.is_c_like()) to catch the as-yet-unimplemented full breadth of the enum type in the Canonical ABI) - but then the load and store implementations in canon_abi_utils return an error, rather than matching the behavior here (i.e. by assuming that the enum is C-like, and thus that we're loading/storing the corresponding discriminant type).
| diagnostics.diagnostic(Severity::Error).with_message(message).into_report() | ||
| })?; | ||
|
|
||
| if cross_ctx_export_sig_flat.params().iter().any(|param| param.ty.is_pointer()) { |
There was a problem hiding this comment.
I believe this is handling the case where specific heap-allocated types that are valid in the Canonical ABI (such as list<T>), but which we do not yet support, are being passed across the component boundary. In the Wasm Component Model, it is left to the Wasm runtime to copy those types from the memory of the caller component to the memory of the callee component - and for our purposes we must emit code which uses the advice provider for that. IIUC, this is just raising a diagnostic to notify the user that support for such types are not yet implemented.
I do think the diagnostic message is misleading though (unless I'm mistaken about why this check is being performed here - but we can't ever allow arbitrary references to caller memory to be passed across a component boundary, with the exception I noted above). It might be better to elaborate on what specifically the issue is (i.e. is it a matter of using something like list<T> when we haven't implemented support for that Canonical ABI type yet - or is it an attempt to pass a pointer across the component boundary, which we will never support)
| Type::U64 => builder.u64(0, SourceSpan::default()), | ||
| Type::F64 => builder.f64(0.0, SourceSpan::default()), | ||
| Type::Felt => builder.felt(Felt::ZERO, SourceSpan::default()), | ||
| Type::Enum(enum_ty) => emit_zero(enum_ty.discriminant(), builder, diagnostics)?, |
There was a problem hiding this comment.
We need to make sure we assert is_c_like on these everywhere that we handle them like this (i.e. by treating the enum as its discriminant type), so that we can catch any misses later when we flesh out enum support for the Canonical ABI
parameters via the advice provider in a cross-context `call`
|
Thank you! I addressed the review notes. Please do another round. |
Close #970
Close #1011
This PR migrates the compiler and SDK stack to the newer Miden VM/runtime APIs, centered around
miden-vm0.22and the corresponding protocol/base updates.On the compiler side, this updates wasm lowering/codegen to match the new VM conventions, including the generalized sign-extension op, corrected little-endian limb handling for 64-bit values, and removal of legacy intrinsics in favor of the newer core/stdlib entry points.
The Miden SDK public API changes in this PR are:
Assetchanges from a single-word layout (inner: Word) to a two-word layout (key: Word,value: Word), andAsset::newnow takes(key, value).asset::build_fungible_asset/asset::build_non_fungible_assetare renamed toasset::create_fungible_asset/asset::create_non_fungible_asset.active_note::get_inputs()is renamed toactive_note::get_storage().input_note::get_inputs_info()is renamed toinput_note::get_storage_info(), andInputNoteInputsInfobecomesInputNoteStorageInfo { commitment, num_storage_items }.active_accountgainsget_assetandget_initial_assetfor direct vault lookups by asset key.AccountId,Tag,NoteIdx, andNoteTypenow convert to/fromWord, andStorageSlotIdaddsto_suffix_prefix().FeltandWordfrommiden_field(miden-core).Triggered by the migration changes in this PR:
i32/i64sign-extension cases.cabi_reallocruntime export required by generated component bindings when values are passed indirectly (Triggered by theAssetbecoming two word size).This PR is quite large. I suggest reviewing it on a per-commit basis, skipping not-important commits.
TODO:
miden-debugmiden-clientin the network testsmiden-clienmigration PR - 0xMiden/miden-client#1891